Improve web unit tests coverage - #162

ui/web/tests/unit/components/features/settings/DeleteAccountPanel.test.tsx (1)

107-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert button recovery after failed deletion.

These tests only verify the error text, so they would still pass if setSubmitting(false) in finally were removed and the button remained stuck at "Deleting...". Assert that each rejection restores an enabled "Delete account" button.

Suggested assertions

     await waitFor(() => {
       expect(screen.getByText("Server error during account deletion")).toBeTruthy()
+      const button = screen.getByRole("button", { name: "Delete account" }) as HTMLButtonElement
+      expect(button.disabled).toBe(false)
     })
...
     await waitFor(() => {
       expect(screen.getByText("Failed to delete account. Please try again.")).toBeTruthy()
+      const button = screen.getByRole("button", { name: "Delete account" }) as HTMLButtonElement
+      expect(button.disabled).toBe(false)
     })

As per coding guidelines, tests should cover the changed behavior and all tests should pass before completion.

Also applies to: 122-124

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/components/features/settings/DeleteAccountPanel.test.tsx`
around lines 107 - 109, Update the failed-deletion test cases around the
existing error assertions to also verify recovery: after each rejected deletion,
assert that the “Delete account” button is present and enabled rather than
remaining in the “Deleting...” state. Cover both rejection paths referenced by
the comment while preserving the existing server-error assertions.

Source: Coding guidelines

ui/web/tests/unit/lib/editor.test.ts (1)

53-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exact-HTML assertions couple the test to SmartPasteService's real markdown renderer output.

These two tests don't mock SmartPasteService.resolvePaste (unlike the "bails out" test at Line 88), so they assert on the literal HTML string produced by the real markdown-to-HTML conversion (e.g. "<p><strong>Bold Text</strong></p>\n"). Any formatting change in the underlying renderer (whitespace, wrapping, etc.) would break these tests even though applySelectionAsMarkdown itself is unchanged. Mocking resolvePaste here too (as done elsewhere in the file) would isolate the unit under test and reduce flakiness risk.

♻️ Example: mock resolvePaste for isolation

   it("converts selected markdown text to HTML, replaces selection, and triggers onContentChange", () => {
     const {
       editor,
       chainMock,
       focusMock,
       deleteRangeMock,
       insertContentMock,
       runMock,
       textBetweenMock,
     } = createMockEditor(0, 14, "**Bold Text**")
     const onContentChange = jest.fn()
+    jest.spyOn(SmartPasteService, "resolvePaste").mockReturnValueOnce({
+      html: "<p><strong>Bold Text</strong></p>\n",
+      type: "markdown",
+      warnings: [],
+      detection: { type: "markdown", confidence: 1, reasons: [], warnings: [] },
+    })

     applySelectionAsMarkdown(editor, onContentChange)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/lib/editor.test.ts` around lines 53 - 82, Mock
SmartPasteService.resolvePaste in both applySelectionAsMarkdown tests so they
verify editor selection replacement and callback behavior without relying on the
renderer’s exact HTML output. Configure the mock with representative converted
content and update the insertContent assertions to use that controlled result,
matching the existing mock pattern in the file.

ui/web/tests/unit/components/EditorMenuBar.test.tsx (1)

1-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

isActive mock branching logic is non-obvious; a short comment would help.

The three-branch dispatch in isActiveMock (object-arg vs. name+attributes vs. plain name) mirrors TipTap's overloaded editor.isActive() signature, but this isn't explained anywhere. A short comment mapping each branch to its real-world call site (e.g. editor.isActive({ textAlign: 'left' }) vs editor.isActive('heading', { level }) vs editor.isActive('bold')) would make future maintenance easier.

As per coding guidelines, "Follow established project code style, use meaningful self-documenting names, and add comments for complex or non-obvious logic."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/components/EditorMenuBar.test.tsx` around lines 1 - 129,
Add a concise comment immediately above isActiveMock's branching logic
explaining that it mirrors TipTap's overloaded editor.isActive calls: object
argument for attributes such as textAlign, name plus attributes for nodes such
as heading levels, and plain name for marks such as bold. Leave the existing
branching behavior unchanged.

Source: Coding guidelines

ui/web/tests/unit/components/features/search/NoteSearchResults.test.tsx (1)

126-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid hardcoding derived chunk length; matches inconsistency with sibling test file.

Line 140 hardcodes 24 as the expected chunkLength argument, coupling the test to the exact output of getRagChunkBodyLength. ChunkSearchItem.test.tsx uses expect.any(Number) for the equivalent assertion — consider the same here for consistency and resilience to unrelated implementation changes.

♻️ Suggested change

-      expect(mockOnOpenInContext).toHaveBeenCalledWith('note-42', 10, 24)
+      expect(mockOnOpenInContext).toHaveBeenCalledWith('note-42', 10, expect.any(Number))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/components/features/search/NoteSearchResults.test.tsx`
around lines 126 - 141, Update the onOpenInContext assertion in the “invokes
onOpenInContext callback when clicking open fragment” test to avoid hardcoding
the derived chunk length; keep the expected note ID and start offset unchanged,
and match the sibling ChunkSearchItem test by accepting any numeric value for
the final argument.

ui/web/tests/unit/hooks/useTagSuggestions.test.ts (1)

43-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Redundant/confusing first assertion.

['rub', 'rust'].filter((t) => defaultTags.includes(t)) always evaluates to ['rust'] (since 'rub' isn't in defaultTags), so it's equivalent to the very next assertion. It adds no coverage and obscures what's actually being tested.

♻️ Proposed simplification

-    expect(result.current).toEqual(['rub', 'rust'].filter((t) => defaultTags.includes(t)))
     expect(result.current).toEqual(['rust'])

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/hooks/useTagSuggestions.test.ts` around lines 43 - 55,
Remove the redundant first expect in the custom minChars test and retain the
direct assertion that result.current equals ['rust']. Keep the test setup and
minChars behavior unchanged.

ui/web/tests/unit/hooks/use-toast.test.tsx (1)

264-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test doesn't actually verify duplicate-timer prevention.

The final assertions (toasts[0].open === false, then toasts.length === 0 after advancing timers) would pass identically whether or not a dedup guard exists for the removal timeout — REMOVE_TOAST filtering by id is a no-op on an already-removed id either way. This test can't distinguish "no duplicate timer" from "duplicate timer without a guard."

♻️ Proposed fix using timer-count assertion

     act(() => {
       result.current.dismiss(createdId)
       result.current.dismiss(createdId)
     })

+    // Only one removal timer should have been scheduled despite two dismiss calls
+    expect(jest.getTimerCount()).toBe(1)
+
     expect(result.current.toasts[0].open).toBe(false)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/hooks/use-toast.test.tsx` around lines 264 - 285,
Strengthen the duplicate-dismiss test around useToast().dismiss by asserting the
scheduled removal timer count immediately after calling dismiss twice. Verify
that only one removal timer is created for the toast, while retaining the
existing state and eventual-removal assertions.

ui/web/tests/unit/components/ImportProgressDialog.test.tsx (1)

157-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Fragile button disambiguation via textContent.

closeButtons.find(btn => btn.textContent === "Close") doesn't reliably distinguish the footer Close button from a default dialog close icon (its sr-only "Close" text would produce the same textContent). It currently works only because of coincidental DOM ordering. The same file's other test (Line 182) uses a more robust !btn.querySelector("svg") filter for the same disambiguation — consider aligning this test with that pattern.

♻️ Suggested alignment

-      const footerBtn = closeButtons.find(btn => btn.textContent === "Close")!
+      const footerBtn = closeButtons.find(btn => !btn.querySelector("svg"))!

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/components/ImportProgressDialog.test.tsx` around lines 157
- 176, Update the Close-button selection in the “renders Close button when
import is complete and calls onClose on click” test to exclude buttons
containing an SVG, matching the existing robust disambiguation pattern in the
nearby test. Remove reliance on exact textContent or DOM ordering while
preserving the click and onClose assertion.

ui/web/tests/unit/components/VirtualNoteList.test.tsx (1)

255-301: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Remove the second-argument assertion from this mock expectation.

Function components receive props as their first argument; any remaining positional arguments are a React implementation detail and should not be part of this contract.

♻️ Suggested simplification

-      expect(customListSpy).toHaveBeenCalledWith(
-        expect.objectContaining({
-          height: 400,
-          itemCount: 1,
-          itemSize: 120,
-          width: "100%",
-          overscanCount: 5,
-        }),
-        undefined
-      )
+      expect(customListSpy.mock.calls[0][0]).toEqual(
+        expect.objectContaining({
+          height: 400,
+          itemCount: 1,
+          itemSize: 120,
+          width: "100%",
+          overscanCount: 5,
+        })
+      )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/web/tests/unit/components/VirtualNoteList.test.tsx` around lines 255 -
301, Update both customListSpy expectations in the “passes custom height and
virtualizer parameters to ListComponent” test to assert only the props object
passed as the first argument; remove the undefined second-argument assertions
while preserving all existing prop checks.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes